Trharris/associated apps #2385
Trharris/associated apps #2385
Conversation
| import { runtime } from '../public/runtime'; | ||
|
|
||
| // Open questions | ||
| // 1. I've re-used `TabInstance` from the public API, does that contain all of the information you and app developers might need? |
There was a problem hiding this comment.
That's great! We should double check the type since it's been a long time since TabInstance was created.
There was a problem hiding this comment.
Yup, please take a look! It looks like it could work, and since it's what is returned by pages.tabs.getTabInstances there's some nice synergy.
There was a problem hiding this comment.
Looked at the object thats returned
It does contain what we need
There was a problem hiding this comment.
The object that is returned by our resolvers has a lot more fields compared to this. This is missing order, removeUrl, appId, and a few more. I think we would need to create a new object.
| // Open questions | ||
| // 1. I've re-used `TabInstance` from the public API, does that contain all of the information you and app developers might need? | ||
| // 2. I didn't see any reason to add a `getTabs` function because `pages.tabs.getTabInstances`. Any reason that won't work for you? | ||
| // 3. I've added an `AppTypes[]` param to `addAndConfigureApp` to allow for the host to show different app types to the user. Very open to changes. |
There was a problem hiding this comment.
We should probably stick to an enum of sorts. Teams does have the concept of categories when it comes apps. We can look into it and try to map them to this.
There was a problem hiding this comment.
That would be great, it's also fine to not put every possible category into this enum to start with. I thought this might be a nice way to make it easy to expand to future scenarios without requiring a bunch of annoying breaking changes on both ends.
| // 1. I've re-used `TabInstance` from the public API, does that contain all of the information you and app developers might need? | ||
| // 2. I didn't see any reason to add a `getTabs` function because `pages.tabs.getTabInstances`. Any reason that won't work for you? | ||
| // 3. I've added an `AppTypes[]` param to `addAndConfigureApp` to allow for the host to show different app types to the user. Very open to changes. | ||
| // 4. I've added empty, private `validate` functions for the threadId and TabInstance. Any validation that is possible will help prevent against |
There was a problem hiding this comment.
This might be difficult to do on the TeamsJS layer. On the TMP side we should obviously do it.
There was a problem hiding this comment.
Yup, I'm not making it a hard requirement but something to please keep in mind: not having validation in TeamsJS for input as the source of a large amount of bugs and pain as we tried to expand this to other hosts. In many cases, the TeamsJS documentation and validation was missing because it was baked into the Teams codebase, which made it very difficult in practice for other hosts that wanted to support functions to figure out what they needed to do, what valid input was, etc.
Validating it in the TMP layer is better than nothing obviously, but validation in TeamsJS can help us protect against different definitions of "legal input" on other hosts, which can be a real challenge for making cross platform applications.
Like I said: it's possible there's no way to do validation in TeamsJS (although at the very least I'd like you to check for things like empty string), but the more validation you can do in this layer (or in the host sdk) the better!
| * | ||
| * @throws TODO: Description of errors that can be thrown from this function | ||
| */ | ||
| export function addAndConfigure(hostEntityId: string, appTypes: AppTypes[]): Promise<TabInstance> { |
There was a problem hiding this comment.
Why call it hostEntityId and not threadId here?
|
|
||
| // Open questions | ||
| // 1. According to Debo, `TabInstance` from the public API looks like it would work. Helen asked Debo to follow up about getting more recent fields added. | ||
| // 2. I didn't see any reason to add a `getTabs` function because `pages.tabs.getTabInstances`. Any reason that won't work for you? |
There was a problem hiding this comment.
getTabInstances may not work for us, I don't think it has been updated to return configurable+static tabs.
For more information about how to contribute to this repo, visit this page.
Description
Proposal for adding a new capability for 1P apps to add associated app tabs to themselves.
Main changes in the PR:
Validation
Validation performed:
Unit Tests added:
<Yes/No>
End-to-end tests added:
<Yes/No>
Additional Requirements
Change file added:
<Yes/No>
Related PRs:
Next/remaining steps:
Screenshots: